CloudWatch EMF Route RequestLifecycleHooks #105

derek-globus · 2023-10-13T15:01:51Z

What?

Added support for RequestLifecycleHook registration to the flask ActionProviderBlueprint class.
Classes may be provided at Blueprint instantiation time to register before, after, or
teardown functionality to wrap all view invocation.
Added a CloudWatchEMFLogger RequestLifecycleHook class.
When attached to an ActionProviderBlueprint, it will emit request count, latency, and
response category (2xxs, 4xxs, 5xxs) count metrics through CloudWatch EMF. Metrics
are emitted both at the aggregate AP and at the individual route level.

Dimension Sets

{ActionProvider: <action-provider-name>}
{ActionProvider: <action-provider-name>, Route: <route-type>}
- <route-type> in this case is provided by the library not the user view func, so it'll be "run", "release", "resume", etc.

Metrics:

Count - Count of invocations against an AP or route
2XXs - Count of invocations against an AP or route resulting in a 2XX
4XXs - Count of invocations against an AP or route resulting in a 4XX
5XXs - Count of invocations against an AP or route resulting in a 5XX
RequestLatency - Duration of invocation handling for an AP or route (in milliseconds)

Why?

Provide an easy mechanism for Action Providers to get usage metrics for both their aggregate Action Provider & specific routes of action providers

Usage

from globus_action_provider_tools.flask import ActionProviderBlueprint
from globus_action_provider_tools.flask.request_lifecycle_hooks import (
    CloudWatchMetricEMFLogger,
)

aptb = ActionProviderBlueprint(
    name="TrackedActionProvider",
    import_name=__name__,
    url_prefix="/tracked",
    provider_description=ap_description,
    request_lifecycle_hooks=[
        CloudWatchMetricEMFLogger(namespace="ActionProviders", action_provider_name="TrackedActionProvider")
    ],
)

Testing

unit tested

sirosen

I know I'm jumping into a naming debate here, but I'd like us to avoid using "middleware" for this, specifically because Flask and Werkzeug already have a type of object they support called a "middleware".
In their case, it's something which wraps a whole app, e.g.

app = app_factory()
app = CORSMiddleware(app, cors_config)

So I'm trying to find an alternative name which means basically the same thing.
"Lifecycle Hooks" or something like that?
Observer? (Not quite right, the hooks can modify data...)
HookCollection?

I'm struggling for a good name, but want to avoid an already-taken framework term.

derek-globus · 2023-10-13T16:15:28Z

I know I'm jumping into a naming debate here, but I'd like us to avoid using "middleware" for this, specifically because Flask and Werkzeug already have a type of object they support called a "middleware".

Yeah I didn't expect that I'd get this one through undebated 😬.

My reasoning for sticking with Middleware was that while flask does reference the term middleware, their "support" for it isn't really support. It's "hey wrap our app with a class that can do stuff and make sure it calls the app as well; we'll conventionally call that thing Middleware!" (ref)

I believe the interface I've exposed (a class that taps into their builtin before_request, after_request, teardown_request hooks) actually fits more precisely with the term middleware.

So I'm trying to find an alternative name which means basically the same thing.
"Lifecycle Hooks" or something like that?
Observer? (Not quite right, the hooks can modify data...)
HookCollection?

RequestLifecycleHooks or APRequestLifeCycleHooks would probably be the only one that makes sense in that vein. Middleware is a more accurate naming, but to your point it may make sense to avoid any hint of conflict w/ Flask.

derek-globus · 2023-10-13T16:34:10Z

tests/test_flask_helpers/app_utils.py

@@ -67,7 +67,7 @@ class Config:
 )


-def test_action_enumeration(
+def mock_action_enumeration_func(


I renamed these functions because they were named with test_ which caused pytest to think they were tests to be run when in fact they are utility callables for other tests to supply to the action provider blueprint as action provider view functions.

Example failure: https://github.com/globus/action-provider-tools/actions/runs/6510610219/job/17684687247?pr=105

ada-globus · 2023-10-13T16:37:44Z

Agree with all of the above—"middleware" feels like the clearest, but the potential for confusion is high in this context. I like RequestLifecycleHooks quite a bit.

derek-globus · 2023-10-13T16:47:16Z

I ran this PR by Stephen Zimmerman & he had the same comment about Middlware as terminology but was content with RequestLifecycleHooks.

Updating in new rev

sirosen · 2023-10-13T16:48:37Z

My reasoning for sticking with Middleware was that while flask does reference the term middleware, their "support" for it isn't really support. It's "hey wrap our app with a class that can do stuff and make sure it calls the app as well; we'll conventionally call that thing Middleware!" (ref)

I believe the interface I've exposed (a class that taps into their builtin before_request, after_request, teardown_request hooks) actually fits more precisely with the term middleware.

I agree with you, but I think the potential for confusion is real enough (e.g. me in 6 months) that we should use a slightly less optimal name because the best name was (rudely!) taken from us.

I'm on the RequestLifecycleHooks bandwagon now.

I'll circle back to do a real content-ful review later, but I think it's all good from a quick scan.
I scoffed at the fmt: off for a tick until I saw what it was wrapping, and felt like "okay, yeah, maybe we don't need to black-format everything". 😉 I'm sure you had a similar judgement which led you to turning it off.

derek-globus · 2023-10-13T16:59:25Z

I scoffed at the fmt: off for a tick until I saw what it was wrapping, and felt like "okay, yeah, maybe we don't need to black-format everything". 😉 I'm sure you had a similar judgement which led you to turning it off.

cough cough flows-logging cough cough

derek-globus · 2023-10-13T17:02:31Z

tests/test_flask_helpers/request_lifecycle_hooks/test_cloudwatch_metrics.py

+    assert emf_log.get("2XXs") == expected_2xxs
+    assert emf_log.get("4XXs") == expected_4xxs
+    assert emf_log.get("5XXs") == expected_5xxs
+    assert type(emf_log.get("RequestLatency")) == float


For some reason on py3.12 windows (exclusively) this request latency comes back as 0.0 (my initial condition was request_latency > 0).

I don't know why that was the case (maybe it's inheriting freeze_time from other tests) but felt comfortable loosening the test to just verify that this is a number rather than a specific number/number range.

Example failure: https://github.com/globus/action-provider-tools/actions/runs/6510955778/job/17685743540

Still reviewing, but I have a comment that might be relevant to this, so I will push that before finishing review.

globus_action_provider_tools/flask/middleware/cloudwatch_metrics.py

ada-globus · 2023-10-13T17:08:40Z

tests/test_flask_helpers/request_lifecycle_hooks/test_cloudwatch_metrics.py

+    assert emf_log.get("2XXs") == expected_2xxs
+    assert emf_log.get("4XXs") == expected_4xxs
+    assert emf_log.get("5XXs") == expected_5xxs
+    assert type(emf_log.get("RequestLatency")) == float


Still reviewing, but I have a comment that might be relevant to this, so I will push that before finishing review.

ada-globus

Looks great! Approving now as I don't have any substantive concerns, but have a couple minor notes for consideration.

globus_action_provider_tools/flask/request_lifecycle_hooks/cloudwatch_metrics.py

…udwatch_metrics.py Co-authored-by: Ada <[email protected]>

…ter()

for more information, see https://pre-commit.ci

CloudWatch EMF Route Middleware

2ebb0e6

derek-globus requested review from ada-globus, jakeglobus, kurtmckee and sirosen as code owners October 13, 2023 15:01

sirosen reviewed Oct 13, 2023

View reviewed changes

derek-globus added 2 commits October 13, 2023 11:17

Resolve mypy

7589b81

Remove test_ from non-test helper callables

e253169

derek-globus commented Oct 13, 2023

View reviewed changes

Renamed middleware -> request-lifecycle-hooks

adb7d0f

derek-globus changed the title ~~CloudWatch EMF Route Middleware~~ CloudWatch EMF Route RequestLifecycleHooks Oct 13, 2023

Only verify that request latency is a number

ffbb75f

derek-globus commented Oct 13, 2023

View reviewed changes

ada-globus reviewed Oct 13, 2023

View reviewed changes

ada-globus approved these changes Oct 13, 2023

View reviewed changes

globus_action_provider_tools/flask/request_lifecycle_hooks/cloudwatch_metrics.py Outdated Show resolved Hide resolved

derek-globus and others added 4 commits October 13, 2023 12:36

Update globus_action_provider_tools/flask/request_lifecycle_hooks/clo…

e291741

…udwatch_metrics.py Co-authored-by: Ada <[email protected]>

Move RequestLatency computation from datetime.now() to time.perf_coun…

74455a1

…ter()

[pre-commit.ci] auto fixes from pre-commit.com hooks

ec0f987

for more information, see https://pre-commit.ci

Rename metric 'Count' -> 'RequestCount'

a2606ad

sirosen approved these changes Oct 16, 2023

View reviewed changes

derek-globus merged commit 09ebd2a into main Oct 16, 2023
12 checks passed

derek-globus deleted the cloudwatch-emf-middleware-sc-26163 branch October 16, 2023 15:28

sirosen mentioned this pull request Oct 19, 2023

Release of version 0.14.0 #112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CloudWatch EMF Route RequestLifecycleHooks #105

CloudWatch EMF Route RequestLifecycleHooks #105

derek-globus commented Oct 13, 2023 •

edited

Loading

sirosen left a comment

derek-globus commented Oct 13, 2023

derek-globus Oct 13, 2023

ada-globus commented Oct 13, 2023

derek-globus commented Oct 13, 2023

sirosen commented Oct 13, 2023

derek-globus commented Oct 13, 2023

derek-globus Oct 13, 2023 •

edited

Loading

ada-globus Oct 13, 2023

ada-globus Oct 13, 2023

ada-globus left a comment •

edited

Loading

CloudWatch EMF Route RequestLifecycleHooks #105

CloudWatch EMF Route RequestLifecycleHooks #105

Conversation

derek-globus commented Oct 13, 2023 • edited Loading

What?

Why?

Usage

Testing

sirosen left a comment

Choose a reason for hiding this comment

derek-globus commented Oct 13, 2023

derek-globus Oct 13, 2023

Choose a reason for hiding this comment

ada-globus commented Oct 13, 2023

derek-globus commented Oct 13, 2023

sirosen commented Oct 13, 2023

derek-globus commented Oct 13, 2023

derek-globus Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

ada-globus Oct 13, 2023

Choose a reason for hiding this comment

ada-globus Oct 13, 2023

Choose a reason for hiding this comment

ada-globus left a comment • edited Loading

Choose a reason for hiding this comment

derek-globus commented Oct 13, 2023 •

edited

Loading

derek-globus Oct 13, 2023 •

edited

Loading

ada-globus left a comment •

edited

Loading